-
Notifications
You must be signed in to change notification settings - Fork 74
MLE-26427 Fixing Polaris issues #1881
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Copyright Validation Results ✅ Valid Files
✅ All files have valid copyright headers! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses Polaris static analysis issues by adding null checks throughout the codebase. The changes focus on defensive programming to prevent potential null pointer exceptions, particularly when handling HTTP responses and JSON operations.
Changes:
- Added null checks for HTTP response objects before accessing their methods
- Added null checks for JSON pointer operations and serialized content
- Added null check for consumer callback before invocation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| OkHttpServices.java | Added defensive null checks for HTTP response objects and response body before accessing their properties and methods |
| IncrementalWriteFilter.java | Added null checks for serialized content and consumer callback to prevent null pointer exceptions |
| ContentExclusionUtil.java | Added null check for JSON pointer last segment before accessing its properties |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int status = response != null ? response.code() : 0; | ||
| if (status == STATUS_NOT_FOUND) { |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When response is null, status is set to 0, but the subsequent logic continues to check specific status codes and may attempt to extract error fields from the null response. Consider adding an early return or throwing an exception when response is null, or ensure that all downstream methods like extractErrorFields() handle null responses gracefully.
| ResponseBody body = response != null ? response.body() : null; | ||
| Object entity = (body != null && body.contentLength() != 0) ? getEntity(body, as) : null; |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The null check for response is repeated multiple times in this method. Since the response is checked at line 622 and subsequent logic depends on it being non-null (line 624 throws an exception for STATUS_NOT_FOUND), the additional null checks at lines 651, 663 become redundant. Consider adding a single null check early in the method or documenting why response could legitimately be null at these points despite earlier checks.
| final String location = response != null ? response.headers().get("Location") : null; | ||
|
|
||
| final List<ClientCookie> cookies = new ArrayList<>(); |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The location and cookies variables are marked as final but their values are only used after the response null check. Since they're declared final, consider moving the declarations inside the null check block or removing the final modifier if they need to be conditionally initialized.
| final String location = response != null ? response.headers().get("Location") : null; | |
| final List<ClientCookie> cookies = new ArrayList<>(); | |
| String location = response != null ? response.headers().get("Location") : null; | |
| List<ClientCookie> cookies = new ArrayList<>(); |
| } | ||
|
|
||
| closeResponse(response); | ||
| if (location == null) throw new MarkLogicInternalException("transaction open failed to provide location"); |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When response is null, location will be null (line 1517), leading to this exception being thrown. However, the error message 'transaction open failed to provide location' is misleading when the actual issue is a null response. Consider checking for null response explicitly before this line and providing a more accurate error message.
| ResponseBody body = response.body(); | ||
| T entity = body.contentLength() != 0 ? getEntity(body, as) : null; | ||
| ResponseBody body = response != null ? response.body() : null; | ||
| T entity = body != null && body.contentLength() != 0 ? getEntity(body, as) : null; |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition 'body != null && body.contentLength() != 0' is repeated in multiple locations (lines 664, 927, 2586, 4846, 5065). Consider extracting this logic into a helper method like 'hasContent(ResponseBody body)' to improve maintainability and ensure consistency.
| // Not sure if the doc can have null content - possibly for a naked properties document? - but if it | ||
| // does, just include it in the write set. |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment expresses uncertainty about when serializedContent can be null ('Not sure if the doc can have null content'). This uncertainty should be resolved through code analysis or documentation review. Update the comment to either explain the specific scenarios where null content occurs or remove the speculative language if the behavior is now understood.
| // Not sure if the doc can have null content - possibly for a naked properties document? - but if it | |
| // does, just include it in the write set. | |
| // If the content cannot be serialized (serializeContent returned null), include the document in the write set unchanged. |
09a5b1e to
1d66359
Compare
Most of these already existed in OkHttpServices, but Polaris decided to report them recently
1d66359 to
ff07804
Compare
Most of these already existed in OkHttpServices, but Polaris decided to report them recently